Skip to content

Initial OpenACC port of mpas_atm_update_bdy_tend #1301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

abishekg7
Copy link
Collaborator

Initial OpenACC port of mpas_atm_update_bdy_tend.

This port is required to keep state and tend variables from LBCs eventually resident on GPUs.

@mgduda mgduda requested review from mgduda and gdicker1 May 6, 2025 23:43
@mgduda mgduda added Atmosphere OpenACC Work related to OpenACC acceleration of code labels May 6, 2025
Copy link
Collaborator

@gdicker1 gdicker1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes make sense and GPU runs of this PR and a reference before these changes match.

I'll hold off on full approval for any changes Michael requests or for any changes to the commit history that may come.

@abishekg7
Copy link
Collaborator Author

Thanks for the review! I'll address the comments, but just wanted to note that this PR may not be a priority for merging to develop with host-device memory transfers consolidated around every call to dynamics. But it will be needed eventually.

abishekg7 added 3 commits May 16, 2025 12:22
Combining some OpenACC loops and parallel regions.

Some whitespace changes.

Removing redundant calls to mpas_pool_get_array.
@abishekg7 abishekg7 force-pushed the atmosphere/port_mpas_atm_update_bdy_tend branch from 8bc1e96 to 057ee7e Compare May 16, 2025 19:21
@abishekg7
Copy link
Collaborator Author

Addressed the review comments. I did have to rebase with the latest develop to get bit identical results (with the latest develop). Also noticing the same for #1315 that I'm working on. Checking if commits from earlier this week are causing some of these differences.

@mgduda mgduda self-requested a review May 16, 2025 20:40
rho_zz(:,:) = rho(:,:) / zz(:,:)
!$acc end kernels

!$acc parallel
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add default(present) to this parallel directive (and also to others below)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done and I've also converted the kernels statement to a parallel region, as it is not a temporary loop. Combining these parallel regions is a little tricky, so I've left them separate as it was.

Converting acc kernels to an acc parallel region, following earlier discussions
to only use kernels directive for temporary code regions.

Using the default(present) clauses for parallel regions to be consistent with
the rest of the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Atmosphere OpenACC Work related to OpenACC acceleration of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants